Fix #2198: Introduce pre push hook for ktlint #2002

anandwana001 · 2020-10-13T04:53:34Z

Explanation

As we know that we have ktlint check in our actions and we also enforce for good formatting code. So, rather than sending a PR and then fixing the ktlint issue, we are introducing a pre-push hook which will run the ktlint before any push.
As per this PR, we want any contributor after opening the codebase we want them to run a script which will copy our hooks script into the .git/hooks folder.

Script to run -> bash scripts/setup.sh

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

rt4914

Tested this and it works correctly. Thanks @anandwana001

Will need to defer to @BenHenning and @vinitamurthi for code correctness.

BenHenning · 2020-10-13T07:57:00Z

I'll need to take a look at this tomorrow my time (it's getting a bit late). @vinitamurthi it might be nice if you could take a look at this first. One issue with the pre-commit hook is that we can't force people to actually install it in the Android workflow, and if you don't install it it's not as useful. :)

scripts/pre-commit

vinitamurthi · 2020-10-13T08:58:22Z

scripts/setup.py

+    if file_exists:
+        print('Pre-commit hook already exists')


The downside to this is that, if we make any changes to the pre-commit hook, it will not get updated to folks who already have the hook installed. Maybe we should not do this and just replace the file even if it exists (Since this script shouldn't be run very often anyway)

now with bash, what we are doing is moving of file.

vinitamurthi · 2020-10-13T09:02:59Z

One issue with the pre-commit hook is that we can't force people to actually install it in the Android workflow, and if you don't install it it's not as useful. :)

That's actually why I thought we should install the hook in a setup script (among other reasons). Today the setup script is not something we can force people to do, but in the future we can actually have people use the setup script to install everything they require to get oppia-android working (particularly, bazel :) )
Today since we only need Android Studio which has gradle along with it, the setup script is an 'extra' script that is not really important. But when we move to Bazel, we can actually have the setup script do the following:

Install Bazel (which makes it essential)
Install Ktlint
Install Buf
Install whatever XML/java linters we chose to use
Install codecoverage libraries if need be
Add the pre-commit hook

And in documentation we basically have the installation steps as: Clone the repo, download Android studio, run this setup script.

BenHenning

Thanks @anandwana001! Left some high-level thoughts.

scripts/pre-commit

scripts/setup.py

scripts/pre-commit

vinitamurthi · 2020-10-20T01:34:34Z

scripts/pre-commit

+echo "********************************"
+echo "Checking code formatting"
+echo "********************************"


I'm being a bit nitpicky (sorry!) - can we have the entire lint check work within a function and then call that function in this script? The reason I ask for that is so that we can cleanly extend this hook if we want to add more checks into it later on

One step further: I think the lint check itself should be its own script that this hook calls (apologies if that's what you meant @vinitamurthi). The pre-commit hook ought to just be a set of commands to run, and having the command separate also lets people use it directly.

Yes that's what I meant sorry I didn't make it clear! We should keep them in separate scripts and the pre-commit hook should have commands to run those scripts one after the other

Sounds good, Thanks @vinitamurthi

BenHenning

Thanks @anandwana001! Left a few more thoughts.

BenHenning · 2020-10-20T06:29:17Z

scripts/pre-commit

+echo "********************************"
+echo "Checking code formatting"
+echo "********************************"


One step further: I think the lint check itself should be its own script that this hook calls (apologies if that's what you meant @vinitamurthi). The pre-commit hook ought to just be a set of commands to run, and having the command separate also lets people use it directly.

rt4914 · 2020-11-10T20:18:30Z

@anandwana001 We discussed that if possible I can resolve ben's comment but even if I do that we will need approval from @BenHenning and therefore I am not resolving his comments. Thanks.

BenHenning · 2020-11-17T02:20:44Z

Thanks @anandwana001. Just had one follow-up regarding the executable bit of the pre-commit file, otherwise this looks good to me.

BenHenning

Thanks @anandwana001! I was about to approve then realized one last thing. :( We should make setup.sh the one-stop-solves-all solution for preparing the codebase. To start, it should set up ktlint and in the future we can add other tools (such as Bazel).

.idea/gradle.xml

scripts/kotlin-lint-check.sh

… introduce-pre-commit-hook-ktlint

FareesHussain

Small doubt regarding how to run ktlint in pre push hook

scripts/kotlin-lint-check.sh

scripts/setup.sh

BenHenning

Thanks @anandwana001! LGTM!

scripts/setup.sh

scripts/kotlin-lint-check.sh

BenHenning · 2020-12-24T03:37:24Z

@anandwana001 just 2 comments to address from me before merging. PTAL.

scripts/setup.sh

anandwana001 added 3 commits October 12, 2020 16:15

added hook

0203dfc

introduce ktlint pre-commit-hook

e590ed5

remove bad file

5d979a6

anandwana001 requested review from BenHenning, rt4914 and vinitamurthi October 13, 2020 04:58

anandwana001 assigned rt4914, vinitamurthi and BenHenning Oct 13, 2020

rt4914 approved these changes Oct 13, 2020

View reviewed changes

rt4914 removed their assignment Oct 13, 2020

vinitamurthi reviewed Oct 13, 2020

View reviewed changes

vinitamurthi removed their assignment Oct 13, 2020

BenHenning requested changes Oct 13, 2020

View reviewed changes

scripts/pre-commit Outdated Show resolved Hide resolved

scripts/setup.py Outdated Show resolved Hide resolved

BenHenning assigned anandwana001 and unassigned BenHenning Oct 13, 2020

replace python script with bash, add auto format

50a85bf

anandwana001 commented Oct 19, 2020

View reviewed changes

scripts/pre-commit Outdated Show resolved Hide resolved

anandwana001 assigned rt4914, vinitamurthi and BenHenning and unassigned anandwana001 Oct 19, 2020

vinitamurthi suggested changes Oct 20, 2020

View reviewed changes

vinitamurthi removed their assignment Oct 20, 2020

BenHenning reviewed Oct 20, 2020

View reviewed changes

BenHenning assigned anandwana001 and unassigned BenHenning Oct 20, 2020

added messge to fix issues

f7402f1

rt4914 removed their assignment Nov 10, 2020

anandwana001 mentioned this pull request Nov 13, 2020

Introduce pre push hook for Bazel #2107

Closed

BenHenning assigned anandwana001 and unassigned BenHenning Nov 17, 2020

anandwana001 mentioned this pull request Nov 17, 2020

Add pre-submit checks file for the project #637

Closed

5 tasks

anandwana001 added 2 commits December 1, 2020 11:33

checking pre-push exec

2266a7f

making pre-push exec

6b3feae

anandwana001 assigned BenHenning Dec 1, 2020

BenHenning requested changes Dec 2, 2020

View reviewed changes

.idea/gradle.xml Outdated Show resolved Hide resolved

scripts/kotlin-lint-check.sh Outdated Show resolved Hide resolved

BenHenning removed their assignment Dec 2, 2020

anandwana001 added 2 commits December 10, 2020 15:35

Merge branch 'develop' of https://github.com/oppia/oppia-android into…

f78fa13

… introduce-pre-commit-hook-ktlint

remove unnecessary change

f6015f6

FareesHussain reviewed Dec 11, 2020

View reviewed changes

scripts/kotlin-lint-check.sh Outdated Show resolved Hide resolved

anandwana001 changed the title ~~Introduce pre push hook for ktlint~~ Fix #2198: Introduce pre push hook for ktlint Dec 13, 2020

anandwana001 added 2 commits December 18, 2020 15:29

setting up ktlint and use it

e587117

nit fix

58d1a77

FareesHussain reviewed Dec 18, 2020

View reviewed changes

scripts/setup.sh Show resolved Hide resolved

anandwana001 commented Dec 18, 2020

View reviewed changes

scripts/setup.sh Outdated Show resolved Hide resolved

anandwana001 assigned BenHenning and unassigned anandwana001 Dec 18, 2020

BenHenning approved these changes Dec 24, 2020

View reviewed changes

scripts/setup.sh Outdated Show resolved Hide resolved

BenHenning reviewed Dec 24, 2020

View reviewed changes

scripts/kotlin-lint-check.sh Outdated Show resolved Hide resolved

BenHenning assigned anandwana001 and unassigned BenHenning Dec 24, 2020

FareesHussain reviewed Dec 30, 2020

View reviewed changes

scripts/setup.sh Outdated Show resolved Hide resolved

using java jar

f2f9bcf

anandwana001 merged commit 53b4a65 into oppia:develop Dec 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2198: Introduce pre push hook for ktlint #2002

Fix #2198: Introduce pre push hook for ktlint #2002

anandwana001 commented Oct 13, 2020 •

edited

Loading

rt4914 left a comment

BenHenning commented Oct 13, 2020

vinitamurthi Oct 13, 2020

anandwana001 Oct 19, 2020

vinitamurthi commented Oct 13, 2020

BenHenning left a comment

vinitamurthi Oct 20, 2020

BenHenning Oct 20, 2020

vinitamurthi Oct 22, 2020

anandwana001 Oct 25, 2020

anandwana001 Oct 29, 2020

BenHenning left a comment

BenHenning Oct 20, 2020

rt4914 commented Nov 10, 2020

BenHenning commented Nov 17, 2020

BenHenning left a comment

FareesHussain left a comment

BenHenning left a comment

BenHenning commented Dec 24, 2020

Fix #2198: Introduce pre push hook for ktlint #2002

Fix #2198: Introduce pre push hook for ktlint #2002

Conversation

anandwana001 commented Oct 13, 2020 • edited Loading

Explanation

Checklist

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinitamurthi commented Oct 13, 2020

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rt4914 commented Nov 10, 2020

BenHenning commented Nov 17, 2020

BenHenning left a comment

Choose a reason for hiding this comment

FareesHussain left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Dec 24, 2020

anandwana001 commented Oct 13, 2020 •

edited

Loading